C/LLVM: Fixes with dict get #1700

Smit-create · 2023-04-12T07:16:02Z

No description provided.

czgdp1807 · 2023-04-12T16:06:52Z

integration_tests/CMakeLists.txt

 RUN(NAME test_dict_09        LABELS cpython llvm c)
-RUN(NAME test_dict_10        LABELS cpython llvm) # TODO: Add support of dict with string in C backend 
+RUN(NAME test_dict_10        LABELS cpython llvm) # TODO: Add support of dict with string in C backend
+RUN(NAME test_dict_11        LABELS cpython c) # TODO: Add LLVM support


We can't move ahead without enabling LLVM backend for any test. Please make it work with LLVM before C.

That's a good point, the LLVM backend is a "lowering" backend, so it ensures our design is good. The C/C++/Julia backends are high level backends, so they don't reveal all issues. WASM is also a lowering backend.

Yep, that sounds right. Thanks, I fixed it.

src/libasr/codegen/llvm_utils.cpp

integration_tests/test_dict_11.py

faze-geek · 2023-04-18T05:41:22Z

Did some testing on this branch. The default value should only be considered when the key is not present in the dictionary.

def test_dict_11():
    d : dict[i32, i32]
    d = {1: 1, 2: 2, 3: 3}
    print(d.get(1, -1))
    print(d.get(3,100)) 

test_dict_11()

(lp) C:\Users\kunni\lpython>python try.py
1
3

(lp) C:\Users\kunni\lpython>src\bin\lpython try.py
-1
100

faze-geek · 2023-04-18T05:52:09Z

Also another strange case I came across -

def test_dict_11():
    d : dict[i32, str]
    d = {1:"1"}
    print(d.get(2,"0"))
    print(d.get(4,"0"))

test_dict_11()

(lp) C:\Users\kunni\lpython>src\bin\lpython try.py
0
KeyError: The dict does not contain the specified key

So should be made robust and tested that it does not give key error sometimes when key is not present in the dictionary. This might be a possible case of #1593 in dict

czgdp1807 · 2023-04-19T04:48:41Z

src/libasr/codegen/llvm_utils.cpp

+        std::pair<std::string, std::string> llvm_key = std::make_pair(
+            ASRUtils::get_type_code(key_asr_type),
+            ASRUtils::get_type_code(value_asr_type)
+        );
+        llvm::Type* value_type = std::get<2>(typecode2dicttype[llvm_key]).second;
+        llvm::Value* result = builder->CreateAlloca(value_type, nullptr);


I think result should only be allocated when def_value is not nullptr. I think you should not mix read_item with the logic of dict.get. Make a new method get_item and call it to implement dict.get. Direct access via [] should be done by read_item (which should be left untouched in this PR).

Presently, [] and dict.get are implemented with the same DictItem node in the ASR and we can re-use the same implementation just to extend it with default. I am not sure if it is a good idea to re-implement the same logic for get_item and read_item which effectively are doing the same job.

I think result should only be allocated when def_value is not nullptr.

It's also used on line 1597 which also makes it more generalized in the case when key_hash is matching but not the keys. This was the bug with the previous implementation where it raised an error for the case where key_hash matches but not the key while it didn't raise any error when the key is totally absent. This is also fixed using the current PR. So, I don't think separating two methods for implementing same logic is a good idea.

Well in this PR itself we have seen (as pointed in #1700 (comment) and #1700 (comment)) how read_item is also affected when you implement dict.get inside it. So de-coupling if-else-if checks

I am not sure if it is a good idea to re-implement the same logic for get_item and read_item which effectively are doing the same job.

If code repetition is the issue then you can solve it by using a macro, or a utility method and call it from read_item and get_item.

Presently, [] and dict.get are implemented with the same DictItem node in the ASR and we can re-use the same implementation just to extend it with default

Won't be the case very soon. We will use IntrinsicFunction to implement, dict.item and dict.get separately.

It's also used on line 1597 which also makes it more generalized in the case when key_hash is matching but not the keys.

In https://github.com/lcompilers/lpython/blob/c935c96b50abeb0553213904a9f96247f6f78421/src/libasr/codegen/llvm_utils.cpp, AFAICT, its only used when def_value != nullptr,

lpython/src/libasr/codegen/llvm_utils.cpp

Lines 1596 to 1598 in c935c96

if (def_value != nullptr) {

LLVM::CreateStore(*builder, LLVM::CreateLoad(*builder, def_value), result);

} else {

Again, my POV is still the same read_item and get_item should be different methods. However they can call common utility methods or macros to execute the common logic. if (some_arg_is_present) and then else always indicate that de-coupling is needed.

This was the bug with the previous implementation where it raised an error for the case where key_hash matches but not the key while it didn't raise any error when the key is totally absent.

Please add a test for it and fix the read_item and get_item methods accordingly.

czgdp1807

LGTM. Before merging, please ensure our dict benchmarks don't degrade due to this PR.

Smit-create · 2023-04-20T11:19:29Z

please ensure our dict benchmarks don't degrade due to this PR.

Slight changes are expected because of an extra if-else check in the last commit which was a bug.

czgdp1807 · 2023-04-21T11:12:54Z

src/libasr/codegen/llvm_utils.cpp

+        });
        llvm::Value* item = llvm_utils->list_api->read_item(value_list, pos,
-                                                        false, module, true);
+                                                        false, module, true);;


This should only happen in safe or debug modes of LPython. In fast mode one shouldn't catch these kind of errors. See, #1139.

I think following the same pattern as raising index errors in lists should work.

Thanks, updated this with even dedicated methods for default and bound checking which leaves the original method untouched (removed some if-else checks) and so its performance will be >= existing performance.

Smit-create requested review from certik and czgdp1807 April 12, 2023 07:16

Smit-create added the c Label for C language related changes label Apr 12, 2023

czgdp1807 marked this pull request as ready for review April 12, 2023 16:06

czgdp1807 requested changes Apr 12, 2023

View reviewed changes

czgdp1807 marked this pull request as draft April 12, 2023 16:07

Smit-create changed the title ~~C: Fixes with dict get~~ C/LLVM: Fixes with dict get Apr 13, 2023

Smit-create added the llvm LLVM related changes label Apr 13, 2023

Smit-create force-pushed the dict_get branch from e21a0a9 to bbc7b04 Compare April 17, 2023 11:05

Smit-create requested a review from czgdp1807 April 17, 2023 15:05

Smit-create marked this pull request as ready for review April 17, 2023 15:21

czgdp1807 requested changes Apr 17, 2023

View reviewed changes

src/libasr/codegen/llvm_utils.cpp Outdated Show resolved Hide resolved

integration_tests/test_dict_11.py Show resolved Hide resolved

czgdp1807 marked this pull request as draft April 17, 2023 16:55

Smit-create force-pushed the dict_get branch 2 times, most recently from 6249f49 to c935c96 Compare April 18, 2023 12:04

Smit-create requested a review from czgdp1807 April 18, 2023 12:46

czgdp1807 requested changes Apr 19, 2023

View reviewed changes

Smit-create force-pushed the dict_get branch 3 times, most recently from ba6af20 to 46cde59 Compare April 20, 2023 06:20

Smit-create marked this pull request as ready for review April 20, 2023 07:00

Smit-create requested a review from czgdp1807 April 20, 2023 07:00

czgdp1807 approved these changes Apr 20, 2023

View reviewed changes

czgdp1807 reviewed Apr 21, 2023

View reviewed changes

czgdp1807 marked this pull request as draft April 21, 2023 11:13

Smit-create requested a review from czgdp1807 April 22, 2023 09:36

czgdp1807 approved these changes Apr 22, 2023

View reviewed changes

Smit-create added 7 commits April 22, 2023 15:44

C: Support dict item with default value

2dabb10

Add tests

9edfe58

LLVM: Consider default value in get

cbb2653

LLVM: Add and update tests

9e5da4c

Refactor and use create_if_else

9960922

Catch key not found error

979a759

Add option for bound checking in dict

a01c275

czgdp1807 force-pushed the dict_get branch from d67e28d to a01c275 Compare April 22, 2023 10:14

czgdp1807 marked this pull request as ready for review April 22, 2023 10:14

czgdp1807 enabled auto-merge (squash) April 22, 2023 10:15

czgdp1807 merged commit ce6cae0 into lcompilers:main Apr 22, 2023

Smit-create deleted the dict_get branch April 22, 2023 10:51

	if (def_value != nullptr) {
	LLVM::CreateStore(builder, LLVM::CreateLoad(builder, def_value), result);
	} else {

C/LLVM: Fixes with dict get #1700

C/LLVM: Fixes with dict get #1700

Uh oh!

Conversation

Smit-create commented Apr 12, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

faze-geek commented Apr 18, 2023

Uh oh!

faze-geek commented Apr 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

czgdp1807 left a comment

Choose a reason for hiding this comment

Uh oh!

Smit-create commented Apr 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

faze-geek commented Apr 18, 2023 •

edited

Loading

Smit-create commented Apr 20, 2023 •

edited

Loading